fix: propagate monitor errors to the frontend #1965

kittaakos · 2023-03-17T15:10:42Z

Motivation

This PR enables clear communication of the problem when starting a monitor process fails.

Shows monitor connection status in the <input>

not-connected (board or port is missing),
connecting (when trying to connect to the monitor port),
connected ✅, and
error (the connection has failed for whatever reason, and the user should select another board or close + reopen the widget after fixing any port issues outside of IDE2)

1508.mp4

Change description

Propagate error from backend to frontend via the monitor ws.
Open the ws connection before creating the monitor duplex. Otherwise, the status update or error cannot reach the frontend.
Fixed monitor widget DI.
Fixed monitor service lifecycle inside the widget. onBeforeAttach happened multiple times on IDE2 start if the widget was previously opened.
Fixed connect when creating the ws connection to the monitor. Previously, the promise might have been resolved before the ws OPEN event.
Moved monitor state to common.
Monitor client proxy waits for reconciled boards after the app startup, then starts the monitor.
Replaced the connected: boolean with a fine-grained connectionStatus and kept the code backward compatible with the plotter app.
Add translation for baud rate.
Smoother monitor widget re-render on monitor reset. (On monitor reset, the monitor widget is forcefully re-rendered causing the <input> and <select>s to disappear for a short time #1985)

1985.mp4

When there is no monitor connection, the <input> is readOnly and keeps the user input. (The monitor <input> is not readOnly when there is no connection to the monitor #1984)

1984.mp4

The monitor gracefully handles when the board's platform is not installed. (Serial monitor throws error when trying to retrieve the enumerateMonitorPortSettings and the platform is not installed #1974)

1974.mp4

Set the monitor widget header color and shifted a few pixels here and there to provide a better monitor UI until a final design is ready as part of Maintain border on Serial Monitor input field when it loses focus #682.

682.mp4

Aligned HC warning colors with the error colors:

2.0.4:

PR:

Other information

Closes #1508
Closes #1985
Closes #1984
Closes #1974
Ref #682

Reviewer checklist

PR addresses a single concern.
The PR has no duplicates (please search among the Pull Requests before creating one)
PR title and description are properly filled.
Docs have been added / updated (for bug fixes / features)

kittaakos · 2023-03-20T08:52:21Z

I have verified the followings:

macOS
- upload to the same board while two connected monitors are running (with Uno and 33 BLE),
- start monitor when another process is using the port, such as Arduino IDE 1.x.
Windows
- upload to the same board while two connected monitors are running (with Uno and Micro),
- start monitor when another process is using the port, such as Arduino IDE 1.x.

I could not upload to 33 BLE on Windows. I restarted my notebook and ran the CMD.exe as an Administrator, but it did not help:

c:\Users\kittaakos\Desktop\cli 0.31.0>arduino-cli.exe version
arduino-cli.exe  Version: 0.31.0 Commit: 940c9457 Date: 2023-02-22T16:15:54Z

c:\Users\kittaakos\Desktop\cli 0.31.0>arduino-cli.exe core list
ID                Installed Latest Name
arduino:avr       1.8.6     1.8.6  Arduino AVR Boards
arduino:mbed_nano 4.0.2     4.0.2  Arduino Mbed OS Nano Boards


c:\Users\kittaakos\Desktop\cli 0.31.0>arduino-cli.exe board list
Port Protocol Type              Board Name          FQBN                        Core
COM3 serial   Serial Port (USB) Arduino Nano 33 BLE arduino:mbed_nano:nano33ble arduino:mbed_nano
COM4 serial   Serial Port (USB) Unknown


c:\Users\kittaakos\Desktop\cli 0.31.0>arduino-cli.exe compile -b arduino:mbed_nano:nano33ble "..\\..\\Desktop\\sketchbooks\\test_sketchbook__2\\monitor__1_1508"
Sketch uses 84680 bytes (8%) of program storage space. Maximum is 983040 bytes.
Global variables use 44560 bytes (16%) of dynamic memory, leaving 217584 bytes for local variables. Maximum is 262144 bytes.

Used platform     Version Path
arduino:mbed_nano 4.0.2   C:\Users\kittaakos\AppData\Local\Arduino15\packages\arduino\hardware\mbed_nano\4.0.2

c:\Users\kittaakos\Desktop\cli 0.31.0>arduino-cli.exe upload -b arduino:mbed_nano:nano33ble -p COM3 "..\\..\\Desktop\\sketchbooks\\test_sketchbook__2\\monitor__1_1508"
TOUCH: error during reset: opening port at 1200bps: Serial port busy
No device found on COM3
Failed uploading: uploading error: exit status 1

c:\Users\kittaakos\Desktop\cli 0.31.0>arduino-cli.exe upload -b arduino:mbed_nano:nano33ble -p COM3 "..\\..\\Desktop\\sketchbooks\\test_sketchbook__2\\monitor__1_1508" -v
Performing 1200-bps touch reset on serial port COM3
TOUCH: error during reset: opening port at 1200bps: Serial port busy
Waiting for upload port...
No upload port found, using COM3 as fallback
"C:\Users\kittaakos\AppData\Local\Arduino15\packages\arduino\tools\bossac\1.9.1-arduino2/bossac.exe" -d --port=COM3 -U -i -e -w "C:\Users\kittaakos\AppData\Local\Temp\arduino\sketches\9374601C8719AF4D46679576E3DD0DC6/monitor__1_1508.ino.bin" -R
No device found on COM3
Failed uploading: uploading error: exit status 1

c:\Users\kittaakos\Desktop\cli 0.31.0>

per1234

When I perform the steps described in #1508 I get an incorrect message in the input field of the Serial Monitor view:

The problem that caused the monitor initialization to fail is that I had the port open in another application. I already have the correct board and port selected, so this "Select a board and a port to connect automatically." advice is wrong.

kittaakos · 2023-03-24T08:11:58Z

The problem that caused the monitor initialization to fail is that I had the port open in another application. I already have the correct board and port selected, so this "Select a board and a port to connect automatically." advice is wrong.

@91volt, please help with how this should work. Thank you!

per1234

When I perform the steps described in #1508, there is a delay of ~15 seconds before the "Port monitor error: command 'open' failed: Serial port busy." notification appears.

The error appears in the logs immediately upon opening the Serial Monitor view:

2023-03-24T07:58:20.085Z monitor-service ERROR Error: 2 UNKNOWN: Port monitor error: command 'open' failed: Serial port busy
    at Object.callErrorFromStatus (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\@grpc\grpc-js\build\src\call.js:31:19)
    at Object.onReceiveStatus (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\@grpc\grpc-js\build\src\client.js:413:49)
    at Object.onReceiveStatus (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\@grpc\grpc-js\build\src\client-interceptors.js:328:181)
    at E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\@grpc\grpc-js\build\src\call-stream.js:187:78
    at processTicksAndRejections (node:internal/process/task_queues:78:11)
for call at
    at ServiceClientImpl.makeBidiStreamRequest (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\@grpc\grpc-js\build\src\client.js:398:30)
    at ServiceClientImpl.monitor (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\@grpc\grpc-js\build\src\make-client.js:105:19)
    at MonitorService.createDuplex (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\arduino-ide-extension\lib\node\monitor-service.js:157:34)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\arduino-ide-extension\lib\node\monitor-service.js:220:37
    at async retry (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\@theia\core\lib\common\promise-util.js:73:20)
    at async MonitorService.start (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\arduino-ide-extension\lib\node\monitor-service.js:135:13)
    at async MonitorManager.startMonitor (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\arduino-ide-extension\lib\node\monitor-manager.js:89:13)
    at async JsonRpcProxyFactory.onRequest (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\@theia\core\lib\common\messaging\proxy-factory.js:132:24)

Then it retries the monitor initialization 9 more times before finally showing the notification.

kittaakos · 2023-03-24T08:23:23Z

The error appears in the logs immediately upon opening the Serial Monitor view:

Nothing new. See here: #1966.

Then it retries the monitor initialization 9 more times before finally showing the notification.

This is precisely the same behavior as on the main with 9b49712. Nothing has changed:

arduino-ide/arduino-ide-extension/src/node/monitor-service.ts

Line 38 in 9b49712

const MAX_WRITE_TO_STREAM_TRIES = 10;

arduino-ide/arduino-ide-extension/src/node/monitor-service.ts

Lines 362 to 375 in 9b49712

    
           // if "pollingIsSuccessful" is false 
        
           // the CLI gave us an error, lets try again 
        
           // after waiting 2 seconds if we've not already 
        
           // reached MAX_WRITE_TO_STREAM_TRIES 
        
           if (pollingIsSuccessful === false) { 
        
             attemptsRemaining -= 1; 
        
             if (attemptsRemaining > 0) { 
        
               setTimeout(startPolling, 2000); 
        
               return; 
        
             } else { 
        
               resolve(false); 
        
               return; 
        
             } 
        
           }

per1234 · 2023-03-24T08:48:05Z

Nothing new.

I didn't intend to imply it was something new. I mentioned it because it shows that the delay does not have an external cause. Arduino IDE knows right away that the monitor initialization failed, but doesn't immediately show the notification.

This is precisely the same behavior as on the main with 9b49712. Nothing has changed:

Something has changed. Arduino IDE now shows a notification that the monitor initialization failed. There is no notification at all on main.

The problem is there is an unnecessarily long delay before the notification appears.

kittaakos · 2023-03-30T14:47:17Z

I made the changes. Please review. Thanks

AlbyIanna

Code looks good to me! I just left some remarks, but they're definitely not blocking.

P.S.: for this review I focused mainly on the code and only did some basic tests of the functionality; let me know if you want me to test it more deeply.

arduino-ide-extension/src/browser/serial/monitor/monitor-widget.tsx

arduino-ide-extension/src/common/protocol/monitor-service.ts

AlbyIanna · 2023-04-03T08:26:21Z

I just left some remarks, but they're definitely not blocking.

Follow up on this: all comments have been addressed. Thank you, Akos!

- Handle when the board's platform is not installed (Closes #1974) - UX: Smoother monitor widget reset (Closes #1985) - Fixed monitor <input> readOnly state (Closes #1984) - Set monitor widget header color (Ref #682) Closes #1508 Signed-off-by: Akos Kitta <a.kitta@arduino.cc>

kittaakos force-pushed the #1508 branch from 1e41d44 to 4e315ab Compare March 17, 2023 15:21

kittaakos added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project topic: serial monitor Related to the Serial Monitor labels Mar 17, 2023

kittaakos self-assigned this Mar 17, 2023

kittaakos force-pushed the #1508 branch 6 times, most recently from 305b1b8 to eb2fc72 Compare March 17, 2023 17:03

kittaakos marked this pull request as ready for review March 20, 2023 08:49

kittaakos requested review from AlbyIanna, davegarthsimpson and per1234 March 20, 2023 08:49

per1234 requested changes Mar 24, 2023

View reviewed changes

kittaakos force-pushed the #1508 branch 2 times, most recently from 12efc24 to a0a3bf4 Compare March 30, 2023 14:45

kittaakos force-pushed the #1508 branch 2 times, most recently from 8d3eb32 to 9b8e941 Compare March 31, 2023 08:45

AlbyIanna approved these changes Mar 31, 2023

View reviewed changes

arduino-ide-extension/src/browser/serial/monitor/monitor-widget.tsx Show resolved Hide resolved

arduino-ide-extension/src/common/protocol/monitor-service.ts Outdated Show resolved Hide resolved

arduino-ide-extension/src/common/protocol/monitor-service.ts Outdated Show resolved Hide resolved

per1234 approved these changes Apr 7, 2023

View reviewed changes

kittaakos force-pushed the #1508 branch from e59be6e to c43d360 Compare April 13, 2023 09:14

kittaakos merged commit 80d5b5a into main Apr 13, 2023

kittaakos deleted the #1508 branch April 13, 2023 09:58

per1234 mentioned this pull request Aug 14, 2023

feat: simplify board and port handling #2165

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: propagate monitor errors to the frontend #1965

fix: propagate monitor errors to the frontend #1965

kittaakos commented Mar 17, 2023 •

edited

Loading

kittaakos commented Mar 20, 2023

per1234 left a comment •

edited

Loading

kittaakos commented Mar 24, 2023

per1234 left a comment

kittaakos commented Mar 24, 2023

per1234 commented Mar 24, 2023

kittaakos commented Mar 30, 2023

AlbyIanna left a comment

AlbyIanna commented Apr 3, 2023

fix: propagate monitor errors to the frontend #1965

fix: propagate monitor errors to the frontend #1965

Conversation

kittaakos commented Mar 17, 2023 • edited Loading

Motivation

Change description

Other information

Reviewer checklist

kittaakos commented Mar 20, 2023

per1234 left a comment • edited Loading

Choose a reason for hiding this comment

kittaakos commented Mar 24, 2023

per1234 left a comment

Choose a reason for hiding this comment

kittaakos commented Mar 24, 2023

per1234 commented Mar 24, 2023

kittaakos commented Mar 30, 2023

AlbyIanna left a comment

Choose a reason for hiding this comment

AlbyIanna commented Apr 3, 2023

kittaakos commented Mar 17, 2023 •

edited

Loading

per1234 left a comment •

edited

Loading